Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(napi): Fix worker threads importing already-loaded NAPI addon #25245

Merged
merged 3 commits into from
Aug 28, 2024
Merged

fix(napi): Fix worker threads importing already-loaded NAPI addon #25245

merged 3 commits into from
Aug 28, 2024

Conversation

nathanwhit
Copy link
Member

Part of #20613.

If a node addon is using the legacy napi_module_register on ctor approach to module registration, we have to store the registered module so that other threads can load the addon (because napi_module_register will only be called once per process).

@devsnek
Copy link
Member

devsnek commented Aug 28, 2024

I think this intersects with the work luca was doing on adding a dlopen cache.

@nathanwhit
Copy link
Member Author

I think this intersects with the work luca was doing on adding a dlopen cache.

Potentially I suppose? But this is specifically a cache for modules registered by napi_register_module on dylib load, and I believe we would need to do this even with a dylib cache.

devsnek
devsnek approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess this is fine until we have the dlopen cache

@nathanwhit nathanwhit merged commit 7dd861a into denoland:main Aug 28, 2024
17 checks passed
@nathanwhit nathanwhit deleted the napi-worker-threads branch August 28, 2024 17:33
lucacasonato pushed a commit that referenced this pull request Aug 29, 2024
@nathanwhit @lucacasonato
fix(napi): Fix worker threads importing already-loaded NAPI addon (#2…
8ed659d 
…5245)

Part of #20613.

If a node addon is using the legacy `napi_module_register` on ctor
approach to module registration, we have to store the registered module
so that other threads can load the addon (because `napi_module_register`
will only be called once per process).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nathanwhit @devsnek